Skip to content

test(retrieval): commit the OHR-Bench eval harness, with rank metrics and per-source-type slicing - #1361

Open
cbcoutinho wants to merge 3 commits into
masterfrom
bench/retrieval-sweep
Open

test(retrieval): commit the OHR-Bench eval harness, with rank metrics and per-source-type slicing#1361
cbcoutinho wants to merge 3 commits into
masterfrom
bench/retrieval-sweep

Conversation

@cbcoutinho

Copy link
Copy Markdown
Owner

The harness that produced notes 390421 / 390460 / 390487 was never merged — it lived on an abandoned worktree branch, which is why note 390460 records the earlier scratchpad as "unrecoverable". Committing it so benchmark numbers stop depending on a branch nobody can find.

Recovered as-is from 1ed0a288, then extended in three ways a fresh 34-cell sweep showed were necessary. Full findings and limits: note 465006; report: https://claude.ai/code/artifact/cb968609-2440-4d4d-abc0-840b33b61b7f

1. Rank-sensitive metrics, beside the page-gated ones — never instead

metrics.score_retrieval is set membership. page_hit/doc_hit ask only whether the gold chunk is present, and page_lcs joins every matching chunk before computing LCS. A reranker that lifts the gold document from rank 8 to rank 1 moves none of them.

Measured on 280 OHR-Bench queries: enabling reranking changed the page_lcs of 12 of them (+0.011, paired Wilcoxon p=0.034) while moving Success@1 by +0.140 — a 24.5% relative gain the official metric reports as marginal. Two rerankers that page_lcs scores identically (0.603 both) are cleanly separated by Success@1 (0.711 vs 0.739).

aggregate now also returns success_at_1, success_at_3, mrr, mean_gold_rank, found, and fmt prints both families on one line.

Success@k divides by all queries, not by the found subset. A query whose gold never surfaced is a failure at rank 1, not an absent sample — the other denominator lets a system that retrieves fewer golds report a better Success@1. Pinned by test_success_denominator_is_all_queries_not_just_found_ones.

2. Per-source-type slicing (the qrels requirement)

Plan rows carry source_type — the Nextcloud content type of the gold document — and the report grows a BY SOURCE TYPE block whenever any row has one.

This is mandatory for a mixed corpus, and the reason is in the code rather than only here: a 20-word Deck card and an 800-token PDF chunk compete in the same Qdrant collection, our own measured density spread across doc types is ~47× (1.2 → 57 chunks/MB), and a system that is excellent on files and useless on cards posts a respectable aggregate — which is the number that gets quoted.

Absent on OHR-Bench (PDFs only): the slice omits itself rather than fabricating a single-bucket comparison that would imply a comparison the corpus cannot support.

Verified on a labelled plan, and it earned its place immediately — the two metric families ranked the types in opposite order:

synthetic type page_lcs Success@1
note 0.414 (worst) 0.700 (best)
file 0.623 (best) 0.530 (worst)
deck_card 0.618 0.625

3. Matched candidate pools (--fetch)

The rerank arm fetched --rerank-pool (50) while the baseline fetched k*2 (20), so a rerank-vs-none A/B compared reordering skill and a 2.5× deeper pool at once. Its measured effect on page_lcs is only −0.002 — so it is not the reason the deltas looked small — but the comparison was not clean, and note 390487's "gain grows with pool size (+0.026@20 → +0.051@50)" is the signature this confound produces.

Registry additions

bge-m3 (self-hosted; what a gateway-free deployment runs) and titan-v1 embedders; bge — the shipped SEARCH_RERANK_MODEL, which had never been benchmarked — and amazon rerankers; strong re-pointed to bedrock/cohere.rerank-v3-5:0 since the gateway no longer serves the openrouter id; 1024/8192 chunk strategies.

Test coverage

tests/unit/test_retrieval_eval_metrics.py — 18 tests (12 recovered, 6 new). The load-bearing one is test_rank_metrics_see_reordering_that_page_lcs_cannot: same result set, different order, page_lcs/page_hit identical, Success@1 flips 1.0 → 0.0. That is the whole argument for the change, as an executable assertion.

Scripts only — no runtime code, no API surface, so no e2e/contract tier applies. ruff, ruff format, ty clean.

Note on reproducibility

Depends on out-of-repo corpora (~/Downloads/OHR-Bench, ~/Software/OHR-Bench/data/qas_v2.json) and tailnet access to the embedding gateway, so it does not run in CI. That is a real limitation, stated in the README rather than hidden. Its value is that the code and the metric port are now reviewable and versioned instead of living on a lost branch.


This PR was generated with the help of AI, and reviewed by a Human

cbcoutinho and others added 3 commits August 19, 2026 17:12
The mis-namespaced-model warning added in #1356 assumed "explicit
SEARCH_RERANK_URL" implies "not the gateway". Pinning the URL to the
gateway's own /v1/rerank is a legitimate configuration — it is how you
fix the endpoint while still using the gateway — and the `local/` routing
prefix is correct there. The warning fired on that working setup, which
is how operators learn to ignore warnings.

Found by running the gateway-free rerank path end to end against a real
login-flow stack.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… and per-source-type slicing

The harness that produced notes 390421/390460/390487 was never merged — it
lived on an abandoned worktree branch, which is why note 390460 records the
earlier scratchpad as "unrecoverable". Committing it so benchmark numbers stop
depending on a branch nobody can find.

Recovered as-is from 1ed0a288, then extended in three ways that a fresh sweep
showed were necessary.

RANK-SENSITIVE METRICS, reported beside the page-gated ones and never instead
of them. `metrics.score_retrieval` is set-membership: `page_hit`/`doc_hit` ask
only whether the gold chunk is present, and `page_lcs` JOINS every matching
chunk before scoring — so a reranker that lifts the gold document from rank 8
to rank 1 moves none of them. On 280 OHR-Bench queries, enabling reranking
changed the page_lcs of 12 of them (+0.011, p=0.034) while moving Success@1 by
+0.140. `aggregate` now also returns success_at_1 / success_at_3 / mrr /
mean_gold_rank / found. Success@k divides by ALL queries, not by the found
subset: a query whose gold never surfaced is a failure at rank 1, and the other
denominator would let a system that retrieves fewer golds score better.

PER-SOURCE-TYPE SLICING. Plan rows carry `source_type` — the Nextcloud content
type of the gold document — and the report grows a BY SOURCE TYPE block
whenever any row has one. Mandatory for a mixed corpus: a 20-word Deck card and
an 800-token PDF chunk compete in the same collection, our own measured density
spread is ~47x, and a system excellent on files and useless on cards posts a
respectable aggregate. Absent on OHR-Bench (PDFs only), where the slice omits
itself rather than inventing a single bucket. Verified on a labelled plan, and
the two metric families immediately ranked the types in OPPOSITE order — which
is the failure mode the slice exists to expose.

MATCHED CANDIDATE POOLS via a new `--fetch`. The rerank arm fetched
`--rerank-pool` (50) while the baseline fetched `k*2` (20), so a rerank-vs-none
A/B compared reordering skill AND a 2.5x deeper pool at once. Its effect on
page_lcs turns out to be only -0.002, but the comparison was not clean, and
note 390487's "gain grows with pool size" is the signature this produces.

Registry additions: bge-m3 and titan-v1 embedders; bge (the SHIPPED
SEARCH_RERANK_MODEL, never previously benchmarked) and amazon rerankers;
`strong` re-pointed to bedrock/cohere.rerank-v3-5:0 since the gateway no longer
serves the openrouter id; 1024/8192 chunk strategies.

Findings and limits: note 465006.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
C Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Claude finished @cbcoutinho's task in 2m 49s —— View job


Review

Todo list

  • Gather context (PR description, changed files list, commit list)
  • Read changed files (config.py, scripts/retrieval_eval/*, tests)
  • Check CLAUDE.md conventions compliance
  • Review metrics.py rank-sensitive logic
  • Review pipeline.py / clients.py / ocr.py / __main__.py
  • Review test coverage
  • Post final review

Note: this sandbox's Bash tool required manual approval for every command
(including git fetch, uv run pytest, even python3 -c ...), which wasn't
granted here, so I could not execute ruff/ty/pytest myself — the review
below is from reading the code and cross-checking it against the 18 tests in
test_retrieval_eval_metrics.py. Worth widening --allowedTools for uv run pytest/ruff if you want me to actually run the suite next time.


Solid PR — the metrics module is the load-bearing part and it's careful:
score_retrieval/aggregate/gold_rank all match the pinned test cases I
traced by hand, and test_rank_metrics_see_reordering_that_page_lcs_cannot /
test_success_denominator_is_all_queries_not_just_found_ones genuinely do pin
the two behaviors the PR description argues for. This is scripts-only
(no @mcp.tool, no API surface), so the BaseResponse/annotations/e2e-contract
gates in CLAUDE.md correctly don't apply, and that's called out explicitly in
the description.

Two things worth a look, both minor:

  1. nextcloud_mcp_server/config.py:1741-1748 (the "don't warn when
    SEARCH_RERANK_URL points at the gateway" fix) — the gateway check is a raw
    string prefix test:

    self.search_rerank_url.startswith(self.embedding_gateway_url.rstrip("/"))

    This has no boundary check, so EMBEDDING_GATEWAY_URL=https://gw.example +
    SEARCH_RERANK_URL=https://gw.example.evil.com/v1/rerank would incorrectly
    be classified as "pointing at the gateway" and suppress the mis-namespaced-model
    warning, since "https://gw.example.evil.com" starts with the literal
    characters "https://gw.example". It's just a diagnostic warning (not a
    security boundary), so low severity, but a urlsplit(...).netloc == comparison
    (or requiring the stripped prefix be followed by / or end-of-string) would be
    more correct, and test_rerank_url_pointing_at_the_gateway_is_quiet doesn't
    cover the near-miss case. Fix this →

  2. scripts/retrieval_eval/README.md — small nits:

    • Says $OUT (./retrieval_eval_out) is "gitignored", but there's no
      retrieval_eval_out (or similar) entry in .gitignore — a default run from
      the repo root would leave it untracked-but-visible in git status, one
      git add -A away from landing in a commit.
    • Lines 62-63 have a stray extra ``` fence pair after the "Registries"
      paragraph, which renders as an empty trailing code block.

Nothing else stood out: the async/anyio usage (create_task_group, CapacityLimiter,
anyio.Lock) follows repo conventions, the retry/backoff in clients.py is
sensible for an unattended multi-hour sweep, and the OCR module's page-boundary
reconstruction (ocr.py:pages_to_extract) correctly mirrors the _extract
contract the chunkers expect. The astrolabe submodule bump is a one-line pointer
update, nothing to review there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant